Skip to content

MPT-21552 derive order asset e2e fixtures from created order#339

Merged
jentyk merged 1 commit into
mainfrom
fix/MPT-21552
Jun 10, 2026
Merged

MPT-21552 derive order asset e2e fixtures from created order#339
jentyk merged 1 commit into
mainfrom
fix/MPT-21552

Conversation

@jentyk

@jentyk jentyk commented Jun 10, 2026

Copy link
Copy Markdown
Member

🤖 AI-generated PR — Please review carefully.

What

Refactor the commerce order asset e2e fixtures and tests (sync and async) to build the
asset request from a real created order instead of hard-coded fixture/config values.

  • order_asset_factory now takes an order argument and derives the asset lines, order id,
    product, and seller from it.
  • created_order moved to the order-level conftest so it is shared by both order and asset
    tests; order_factory now also emits an asset line.
  • Both the sync and async created_order_asset fixtures build their request from created_order.
  • The asset tests now scope their requests by created_order.id (the order the asset actually
    belongs to) instead of the hard-coded commerce_asset_draft_order_id. The endpoint is
    /public/v1/commerce/orders/{order_id}/assets, so the old scope pointed at a different order
    and caused 404s (and test_filter_order_assets to fail its len(result) == 1 assertion).

Why

Keeps the asset fixture data consistent with the order it belongs to and avoids drift between
separately maintained fixtures.

Testing

  • pre-commit (ruff, flake8 / wemake-python-styleguide, mypy) passes on the changed files.
  • e2e collection succeeds (16 tests). The full e2e suite requires live MPT API credentials and
    runs in CI.

Refs MPT-21552.

Closes MPT-21552

  • Refactored order_asset_factory to accept an order argument and derive asset lines, order ID, product, and seller from the provided order instead of accepting multiple hard-coded ID fixtures
  • Moved created_order fixture from tests/e2e/commerce/order/test_sync_order.py to tests/e2e/commerce/order/conftest.py to be shared between order and asset tests
  • Updated order_factory to include an asset line in order payloads, ensuring orders created for testing contain both commerce and asset items
  • Updated both sync and async created_order_asset fixtures to construct asset requests from the shared created_order fixture
  • Scoped asset endpoint queries to use created_order.id instead of hard-coded commerce_asset_draft_order_id, resolving 404 errors and test failures
  • Updated all asset-related test functions to accept created_order and scope asset operations through the created order
  • Updated Flake8 per-file-ignores configuration for tests/e2e/commerce/order/*.py and tests/e2e/commerce/order/asset/*.py to handle style violations in refactored code

@jentyk jentyk requested a review from a team as a code owner June 10, 2026 16:16
@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR refactors the E2E test fixtures for order assets by consolidating the created_order fixture into shared conftest, extending order_factory to include asset line items, refactoring order_asset_factory to work with created order structures, and updating both async and sync asset tests to use the new fixture layout.

Changes

Order Asset Test Fixture Consolidation

Layer / File(s) Summary
Shared order fixture and asset line integration
tests/e2e/commerce/order/conftest.py, tests/e2e/commerce/order/test_sync_order.py
The created_order fixture is moved from test_sync_order.py to shared conftest.py. The order_factory fixture is extended to accept asset_item_id and now generates a second asset line item in orders alongside the commerce line, using the same pricing structure and quantity.
Order asset factory refactor
tests/e2e/commerce/order/asset/conftest.py
The order_asset_factory signature is simplified to accept no outer parameters, returning a factory function that takes an order dict. The factory filters asset line items from the order and derives identifiers from the order structure instead of using external fixture parameters.
Async asset tests refactor
tests/e2e/commerce/order/asset/test_async_asset.py
The created_order_asset fixture is updated to generate asset payloads from created_order and scope operations via orders.assets(created_order.id). All asset tests (test_get_order_asset_by_id, test_list_order_assets, test_get_order_asset_by_id_not_found, test_filter_order_assets, test_update_order_asset, test_delete_order_asset, test_render_order_asset) are updated to depend on created_order and scope asset endpoint calls accordingly.
Sync asset tests refactor
tests/e2e/commerce/order/asset/test_sync_asset.py
The created_order_asset fixture is updated to generate asset payloads from created_order and scope operations via orders.assets(created_order.id). All asset tests are updated to depend on created_order and scope asset endpoint calls to orders.assets(created_order.id).
Flake8 configuration update
pyproject.toml
Flake8 per-file-ignores are updated, adding WPS211 to tests/e2e/commerce/order/*.py and adding WPS221 to tests/e2e/commerce/order/asset/*.py.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes


Possibly related PRs

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Pr And Commit Formatting ✅ Passed PR title follows format (MPT-21552), description explains changes. Commit follows Conventional Commits, has detailed body, linear history with no merge commits.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

Refactor the commerce order asset e2e fixtures to build the asset
request from a real created order instead of many hard-coded fixture
values. `order_asset_factory` now takes an `order` argument and reads
its lines, id, product, and seller, and `created_order` is moved to the
order-level conftest so it can be shared by both order and asset tests.
Both the sync and async asset suites build their `created_order_asset`
fixture from the created order, and the asset tests scope their requests
by the created order's id instead of a hard-coded draft order id, so they
operate on the order the asset actually belongs to. This keeps the asset
fixture data consistent with the order it belongs to and avoids drift
between separately maintained fixtures.

Refs MPT-21552.
@sonarqubecloud

Copy link
Copy Markdown

coderabbitai[bot]
coderabbitai Bot previously requested changes Jun 10, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/e2e/commerce/order/asset/conftest.py`:
- Line 27: Replace the fragile name-based filter for asset_lines with a stable
check for the technical marker: build asset_lines using [line for line in
order["lines"] if line.get("period") == "one-time"] (or line.get("item",
{}).get("period") if period sits under item) instead of "Asset" in
line["item"]["name"], and add a fail-fast assertion (e.g., assert asset_lines,
"no asset lines found") so the test errors clearly if none are present.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 96b913e1-0d85-48d5-8280-9b8685b29a77

📥 Commits

Reviewing files that changed from the base of the PR and between b297a05 and 1a62be1.

📒 Files selected for processing (6)
  • pyproject.toml
  • tests/e2e/commerce/order/asset/conftest.py
  • tests/e2e/commerce/order/asset/test_async_asset.py
  • tests/e2e/commerce/order/asset/test_sync_asset.py
  • tests/e2e/commerce/order/conftest.py
  • tests/e2e/commerce/order/test_sync_order.py
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • softwareone-platform/mpt-extension-skills (manual)
💤 Files with no reviewable changes (1)
  • tests/e2e/commerce/order/test_sync_order.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build
🧰 Additional context used
📓 Path-based instructions (2)
**/*

⚙️ CodeRabbit configuration file

**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved

Files:

  • pyproject.toml
  • tests/e2e/commerce/order/conftest.py
  • tests/e2e/commerce/order/asset/test_async_asset.py
  • tests/e2e/commerce/order/asset/conftest.py
  • tests/e2e/commerce/order/asset/test_sync_asset.py
**

⚙️ CodeRabbit configuration file

**: # AGENTS.md

Working protocol for any task in this repository:

  1. Identify the task type and select only the local repository files that are relevant to that task.
  2. Read only those relevant local files before making changes.
  3. If any selected local file references shared standards or shared operational guidance that are relevant to the same task, read those shared documents too before proceeding.
  4. Treat repository-local documents as repository-specific additions, restrictions, or overrides to shared guidance.
  5. If a repository-local rule conflicts with a shared rule, the local repository rule takes precedence.

Python API client for the SoftwareONE Marketplace Platform (MPT) API. Provides synchronous
(MPTClient) and asynchronous (AsyncMPTClient) clients built on httpx, with typed
resource services, mixin-based HTTP operations, and an RQL query builder.

Documentation Reading Order

When applicable, read the repository documentation in this order:

  1. README.md — repository overview, quick start, and documentation map
  2. docs/usage.md — installation, configuration, Python usage examples, and supported Docker-based commands
  3. docs/architecture.md — layered architecture, directory structure, and key abstractions
  4. docs/local-development.md — Docker-only setup and execution model
  5. docs/testing.md — repository-specific testing strategy and command mapping
  6. docs/contributing.md — repository-specific workflow and links to shared standards
  7. docs/documentation.md — repository-specific documentation rules

Then inspect the code paths relevant to the task:

  • mpt_api_client/mpt_client.py — public sync and async client entry points
  • mpt_api_client/http/ — HTTP clients, services, query state, and reusable mixins
  • mpt_api_client/resources/ — domain resource groups such as catalog, commerce, billing, and integration
  • mpt_api_client/models/ — response model layer and collection wrappers
  • mpt_api_client/rql/ — fluent RQL query ...

Files:

  • pyproject.toml
  • tests/e2e/commerce/order/conftest.py
  • tests/e2e/commerce/order/asset/test_async_asset.py
  • tests/e2e/commerce/order/asset/conftest.py
  • tests/e2e/commerce/order/asset/test_sync_asset.py
🧠 Learnings (8)
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.

Applied to files:

  • tests/e2e/commerce/order/conftest.py
  • tests/e2e/commerce/order/asset/test_async_asset.py
  • tests/e2e/commerce/order/asset/conftest.py
  • tests/e2e/commerce/order/asset/test_sync_asset.py
📚 Learning: 2026-04-02T09:35:03.825Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 269
File: tests/e2e/helpdesk/chats/links/test_sync_links.py:18-18
Timestamp: 2026-04-02T09:35:03.825Z
Learning: In this repository’s test suite, flake8-aaa/flake8-aaa codes use short two-digit suffixes (e.g., `# noqa: AAA01`), not three-digit variants like `AAA001`. If you see `# noqa: AAA01` in a test file (e.g., when the Act step is performed via a pytest fixture rather than inline code), treat it as valid and intentional—do not flag it as an invalid/no-longer-needed noqa, and do not suggest removing it even if Ruff reports `RUF102`, since `AAA` is configured under `tool.ruff.lint.external` and these noqa directives are expected to be preserved.

Applied to files:

  • tests/e2e/commerce/order/conftest.py
  • tests/e2e/commerce/order/asset/test_async_asset.py
  • tests/e2e/commerce/order/asset/conftest.py
  • tests/e2e/commerce/order/asset/test_sync_asset.py
📚 Learning: 2026-01-08T08:34:05.465Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/conftest.py:29-30
Timestamp: 2026-01-08T08:34:05.465Z
Learning: In end-to-end tests (e.g., tests/e2e/...), reuse existing API resources for read-only operations and safe mutations to speed up test execution. Reserve isolated fixtures (e.g., created_price_list) for destructive tests that require per-test creation and cleanup. Ensure tests document when a fixture creates/destroys data and clearly indicate which operations are destructive, so tests stay fast and properly isolated.

Applied to files:

  • tests/e2e/commerce/order/conftest.py
  • tests/e2e/commerce/order/asset/test_async_asset.py
  • tests/e2e/commerce/order/asset/conftest.py
  • tests/e2e/commerce/order/asset/test_sync_asset.py
📚 Learning: 2026-01-08T23:38:19.565Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 186
File: tests/e2e/billing/ledger/charge/test_sync_ledger_charge.py:33-39
Timestamp: 2026-01-08T23:38:19.565Z
Learning: In Python e2e tests under tests/e2e, hardcoded external IDs (e.g., INV12345) are intentional because they come from seeded test data. Ensure the test data seeds consistently include these IDs; if seeds change, update tests accordingly. Prefer using a named constant for such IDs (e.g., INV_EXTERNAL_ID) and document the dependency on seed data to avoid brittle tests.

Applied to files:

  • tests/e2e/commerce/order/conftest.py
  • tests/e2e/commerce/order/asset/test_async_asset.py
  • tests/e2e/commerce/order/asset/conftest.py
  • tests/e2e/commerce/order/asset/test_sync_asset.py
📚 Learning: 2026-02-02T13:05:41.144Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 201
File: tests/unit/resources/accounts/mixins/test_activatable_mixin.py:132-139
Timestamp: 2026-02-02T13:05:41.144Z
Learning: In the mpt-api-python-client repository, tests are configured to use pytest asyncio mode auto, which auto-detects async test functions and runs them without requiring pytest.mark.asyncio. Reviewers should rely on this behavior for all Python test files under tests/, and avoid adding unnecessary asyncio markers in async tests. Ensure test files in tests/ adhere to this convention unless a specific test requires an explicit marker.

Applied to files:

  • tests/e2e/commerce/order/conftest.py
  • tests/e2e/commerce/order/asset/test_async_asset.py
  • tests/e2e/commerce/order/asset/conftest.py
  • tests/e2e/commerce/order/asset/test_sync_asset.py
📚 Learning: 2026-04-02T16:26:46.138Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 275
File: tests/e2e/exchange/currencies/conftest.py:43-43
Timestamp: 2026-04-02T16:26:46.138Z
Learning: In this repo’s Python E2E conftest files, line-specific flake8/wemake suppressions like `# noqa: WPS421` placed inline on individual statements (e.g., on specific `print()` calls in teardown blocks) are intentional and acceptable. When reviewing, avoid suggesting converting these to a file-wide ignore (e.g., adding per-file ignores under `[tool.flake8]` in `pyproject.toml`), since that would over-suppress WPS421 for the entire file. Also treat Ruff `RUF102` about an unknown rule code as expected here when suppressing `WPS421` from wemake-python-styleguide.

Applied to files:

  • tests/e2e/commerce/order/conftest.py
  • tests/e2e/commerce/order/asset/conftest.py
📚 Learning: 2026-04-06T09:03:34.466Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 277
File: tests/e2e/extensibility/extensions/conftest.py:37-40
Timestamp: 2026-04-06T09:03:34.466Z
Learning: In this repo’s E2E test conftest teardown fixtures, it is intentional to catch `MPTAPIError` broadly (not just 404) and to print a diagnostic message instead of re-raising. This avoids failing the test suite during teardown when a resource cannot be deleted in some scenarios. Do not recommend narrowing the `except` clause in these teardown blocks (e.g., checking `error.status == 404`), unless the fixture is clearly not intended as an E2E teardown diagnostic.

Applied to files:

  • tests/e2e/commerce/order/conftest.py
  • tests/e2e/commerce/order/asset/conftest.py
📚 Learning: 2026-04-16T13:00:41.320Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 303
File: tests/e2e/helpdesk/chats/participants/conftest.py:25-31
Timestamp: 2026-04-16T13:00:41.320Z
Learning: In mpt-api-python-client, do not treat list-wrapped arguments to CreateMixin.create() / AsyncCreateMixin.create() as an error. ResourceData is intentionally typed as Resource | list[Resource] (see mpt_api_client/models/model.py), so create() should accept either a single resource dict or a list of resource dicts (e.g., create([chat_participant_data])) to perform a batch create and return a ModelCollection. Therefore, reviewers should only flag list-wrapped create() arguments when there is evidence they violate the expected API contract beyond this documented batch-create behavior.

Applied to files:

  • tests/e2e/commerce/order/conftest.py
  • tests/e2e/commerce/order/asset/test_async_asset.py
  • tests/e2e/commerce/order/asset/conftest.py
  • tests/e2e/commerce/order/asset/test_sync_asset.py
🔇 Additional comments (4)
pyproject.toml (1)

138-139: LGTM!

tests/e2e/commerce/order/conftest.py (1)

10-15: LGTM!

Also applies to: 20-25, 64-78

tests/e2e/commerce/order/asset/test_async_asset.py (1)

29-33: LGTM!

Also applies to: 37-42, 46-50, 57-60, 66-70, 90-99, 107-114, 120-126

tests/e2e/commerce/order/asset/test_sync_asset.py (1)

29-34: LGTM!

Also applies to: 38-40, 47-50, 58-61, 67-70, 91-99, 108-114, 121-126

Comment thread tests/e2e/commerce/order/asset/conftest.py
@jentyk jentyk dismissed coderabbitai[bot]’s stale review June 10, 2026 17:50

requested change resolved

@jentyk

jentyk commented Jun 10, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai approve

@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown
✅ Action performed

Comments resolved and changes approved.

@jentyk jentyk merged commit 2dc276c into main Jun 10, 2026
4 checks passed
@jentyk jentyk deleted the fix/MPT-21552 branch June 10, 2026 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants